Prevent redundant PaymentClaimed events for closed channels#4467
Prevent redundant PaymentClaimed events for closed channels#4467valentinewallace wants to merge 5 commits intolightningdevkit:mainfrom
PaymentClaimed events for closed channels#4467Conversation
Useful in the next commit because we'll need to repurpose the same handling code for a new monitor update that is similar to ReleasePaymentComplete, but for inbound payments to avoid regenerating redundant PaymentClaimed events.
When an Event::PaymentClaimed is processed by the user, we need to track that so we don't keep regenerating the event redundantly on startup. Here we add a monitor update for that. Note that this update will only be generated for closed channels -- if the channel is open, the inbound payment is pruned automatically when the HTLC is no longer present in any unrevoked commitment transaction, which stops the redundant event regeneration. Upcoming commits will add generation and handling of this monitor update
When an Event::PaymentClaimed is processed by the user, we need to track that so we don't keep regenerating the event redundantly on startup. Here we add a map to claims in this state, similar to the existing htlcs_resolved_to_user map. Note that this map will only be updated for closed channels -- if the channel is open, the inbound payment is pruned automatically when the HTLC is no longer present in any unrevoked commitment transaction, which stops the redundant event regeneration. Upcoming commits will add the actual tracking that uses this map, as well as generating the relevant monitor update to trigger this tracking
When an Event::PaymentClaimed is processed by the user, we need to track that so we don't keep regenerating the event redundantly on startup. Here we add an event completion action for that. Note that this action will only be generated for closed channels -- if the channel is open, the inbound payment is pruned automatically when the HTLC is no longer present in any unrevoked commitment transaction, which stops the redundant event regeneration. Similar to EventCompletionAction::ReleasePaymentCompleteChannelMonitorUpdate, but for inbound payments. Actual generation and handling of this completion action is added in an upcoming commit.
Previously, if we had an inbound payment on a closed channel that we started claiming but did not end up removing from the commitment tx, we would generate a PaymentClaimed event for the payment on every restart until the channelmonitor was archived. The past few commits have laid the groundwork to get rid of this redundancy -- here we now generate an event completion action for when the PaymentClaimed is processed by the user, at which point a monitor update will be released that tells the channel monitor that the user has processed the PaymentClaimed event. The monitor tracks this internally such that the pending claim will no longer be returned to the ChannelManager when reconstructing the set of mid-claim inbound payments on startup, and thus no longer generate the redundant event.
|
👋 Hi! This PR is now in draft status. |
| @@ -3249,6 +3270,20 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> { | |||
|
|
|||
| pub(crate) fn get_stored_preimages( | |||
There was a problem hiding this comment.
Not sure if we want to also rename this method
| // behavior (RAA updates are blocked until the PaymentClaimed event is handled). | ||
| // For closed channels, we use InboundPaymentClaimedChannelMonitorUpdate to persist | ||
| // that the PaymentClaimed event has been handled, preventing regeneration on restart. | ||
| if is_channel_closed { |
There was a problem hiding this comment.
I went with only generating these new monitor updates for claims on closed channels. So after #4462 we will still potentially have increased redundant PaymentClaimed events on restart for claims on open channels, until the claim is removed from all unrevoked commit txs. The approach is currently simple so I'm not sure it's worth fixing that, but open to thoughts.
| @@ -3249,6 +3270,20 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> { | |||
|
|
|||
| pub(crate) fn get_stored_preimages( | |||
| pub(crate) fn get_stored_preimages( | ||
| &self, | ||
| ) -> HashMap<PaymentHash, (PaymentPreimage, Vec<PaymentClaimDetails>)> { | ||
| let inner = self.inner.lock().unwrap(); |
There was a problem hiding this comment.
Hmm, can we do this on a per-PaymentClaimDetails basis instead? In theory we can have two payments with the same hash/preimage.
| (34, alternative_funding_confirmed, option), | ||
| (35, is_manual_broadcast, (default_value, false)), | ||
| (37, funding_seen_onchain, (default_value, true)), | ||
| (39, inbound_payments_claimed, option), |
There was a problem hiding this comment.
Assuming that write as required but read as option is intentional to be forwards compatible for old monitors?
| // For closed channels, we use InboundPaymentClaimedChannelMonitorUpdate to persist | ||
| // that the PaymentClaimed event has been handled, preventing regeneration on restart. |
There was a problem hiding this comment.
perhaps too edge-casey to worry about(?): durable_preimage_channel is picked as the last MPP channel, if this happens to be open and another one is closed do we still run into the duplicate events issue?
There was a problem hiding this comment.
That's a good point. It's tricky to fix, unfortunately, since we only have the OutPoint for the durable channel and we need that field to support downgrades to 0.1-. If we went for a more robust fix here and figured out backwards compat to persist outpoints for all channels, it seems like the simplest way would be to always issue this new monitor update for every MPP part when the event is processed, which is kind of annoying.
Since this fix is already not fully robust, only an improvement on the previous situation, my preference would be to just keep it as-is at least for now.
There was a problem hiding this comment.
and we need that field to support downgrades to 0.1
Was wondering if this is still supported, bc there's a comment about using htlcs instead once we don't. Happy to leave as-is 👌
There was a problem hiding this comment.
Thanks, yeah, I mean if people feel like this fix is too narrow and we should just accept redundant events until the channel is archived, I could honestly get behind that as well.
4 participants
Previously, if we had an inbound payment on a closed channel that we started
claiming but did not end up removing from the commitment tx, we would generate
a
PaymentClaimedevent for the payment on every restart until thechannelmonitor was archived.
Becomes more important with #4462 -- currently we rely on checking whether a payment is present in
ChannelManager::pending_claiming_paymentsto prevent some redundant event generation, but once we start always reconstructing that map we can no longer perform those checks.